Skip to content

Update namespace & topic admin methods to return nil if unset #1433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
crossoverJie merged 5 commits into from
Oct 30, 2025
Merged

Update namespace & topic admin methods to return nil if unset #1433

crossoverJie merged 5 commits into from
Oct 30, 2025

Conversation

@klevy-toast
Copy link
Contributor

@klevy-toast klevy-toast commented Oct 28, 2025 •
edited
Loading

Fixes #1431

Motivation

For the namespace / topic admin methods that return a pointer, explicitly return nil if the method isn't set (API returns empty body)

Modifications

  • New method in client.go GetBodyWithContext that returns body & error
  • Modify 14 methods across topic & namespace files to use new method, check the body, and return nil if the body is nil
  • Update tests and add new test coverage that was missing

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added missing test coverage for properties that were not previously tested
  • Modified existing test to verify property is nil on initial state / after removal

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • Updated the go docs to mention nil return values on all relevant methods

@klevy-toast klevy-toast mentioned this pull request Oct 28, 2025
Closed
@RobertIndie RobertIndie added this to the v0.18.0 milestone Oct 28, 2025
@lhotari
Copy link
Member

lhotari commented Oct 28, 2025

test failure:

=== RUN   TestGetTopicAutoCreation
    namespace_test.go:159: 
        	Error Trace:	/pulsar/pulsar-client-go/pulsaradmin/pkg/admin/namespace_test.go:159
        	Error:      	Expected value not to be nil.
        	Test:       	TestGetTopicAutoCreation
        	Messages:   	Expected non-nil when topic auto creation is configured
--- FAIL: TestGetTopicAutoCreation (0.01s)

@klevy-toast klevy-toast marked this pull request as draft October 28, 2025 18:45
@klevy-toast klevy-toast marked this pull request as ready for review October 28, 2025 20:15
@klevy-toast
Copy link
Contributor Author

klevy-toast commented Oct 28, 2025

@lhotari Thanks, fixed up the test issues. I needed to change a bit more in the client package that should be reviewed carefully.

@crossoverJie crossoverJie requested a review from Copilot October 29, 2025 02:02
Copilot AI reviewed Oct 29, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR modifies the behavior of topic and namespace policy getter methods to return nil when policies are not configured at the topic/namespace level, rather than returning default/zero values. This is accomplished by introducing a new GetBodyWithContext method and decodeJSONWithBody helper that detect empty HTTP response bodies.

Key Changes

  • Added GetBodyWithContext and decodeJSONWithBody functions to REST client for detecting empty response bodies
  • Updated policy getter methods to return nil when policies are not configured (empty response)
  • Updated tests to expect nil values instead of zero-value structs when policies are removed or not configured

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pulsaradmin/pkg/rest/client.go Added GetBodyWithContext method and decodeJSONWithBody helper to detect and return empty response bodies
pulsaradmin/pkg/admin/topic.go Updated policy getter methods and documentation to return nil when policies are not configured
pulsaradmin/pkg/admin/topic_test.go Updated tests to expect nil instead of zero values when policies are not configured
pulsaradmin/pkg/admin/namespace.go Updated namespace policy getter methods and documentation to return nil when policies are not configured
pulsaradmin/pkg/admin/namespace_test.go Added comprehensive tests for namespace policies expecting nil for unconfigured policies
Comments suppressed due to low confidence (1)

pulsaradmin/pkg/rest/client.go:220

  • Line 220 returns nil, err but err is undefined at this point if the else if !decode block was entered but neither the file != nil nor the else branch returned. This will cause a compilation error or return an undefined error. Should return nil, nil instead.
	return nil, err

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crossoverJie
crossoverJie approved these changes Oct 29, 2025
View reviewed changes
Copy link
Member

@crossoverJie crossoverJie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crossoverJie crossoverJie merged commit 50efe42 into apache:master Oct 30, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@crossoverJie crossoverJie crossoverJie approved these changes

@RobertIndie RobertIndie RobertIndie approved these changes

@freeznet freeznet Awaiting requested review from freeznet

Assignees

@klevy-toast klevy-toast

Labels

release/important-notice type/enhancement

Projects

None yet

Milestone

v0.18.0

Development

Successfully merging this pull request may close these issues.

Admin client methods that return pointers lead to ambiguous behavior for unset vs 0 values

4 participants

@klevy-toast @lhotari @crossoverJie @RobertIndie